[Kernel]Remove two file IO in CRC loading by reusing log listing #4112

huan233usc · 2025-02-01T04:18:35Z

Which Delta project/connector is this regarding?

Description

Incremental CRC loading for P&M was added in #4077.

For snapshot N, we try to read N.crc, if N.crc missing, we list CRC files and find the one up to N-100. These two file listing operation could be actually avoided, because we have already listed files under _delta_log folder.

This PR collects latest CRC in the first file listing and remove two unnecessary IO.

How was this patch tested?

LogReplay test

Does this PR introduce any user-facing changes?

No

scottsand-db · 2025-02-13T21:19:39Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/DeltaLogActionUtils.java


    // Must be final to be used in lambda
-    final AtomicBoolean hasReturnedAnElement = new AtomicBoolean(false);
+    final AtomicBoolean hasReturnedlogOrCheckPoint = new AtomicBoolean(false);


hasReturnedCommitOrCheckpoint

You can add a comment here. Something like: This variable is used to help determine if we should throw an error if the table history is not reconstructable. Only commit and checkpoint files are applicable.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/DeltaLogActionUtils.java

scottsand-db · 2025-02-13T21:24:00Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/DeltaLogActionUtils.java

              }

-              hasReturnedAnElement.set(true);
+              // Only log and checkpoint could use to construct table state.


You can delete this comment line

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java

scottsand-db · 2025-02-13T21:31:38Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java

        newVersion,
        deltasAfterCheckpoint,
        latestCompleteCheckpointFileStatuses,
+        listedChecksumFileStatues.stream()


wouldn't it just be the last one?

yes, updated

...el/kernel-defaults/src/test/scala/io/delta/kernel/defaults/LogReplayEngineMetricsSuite.scala

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/LogSegment.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/LogReplay.java

scottsand-db · 2025-02-13T21:45:31Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/LogSegment.java

        + list.stream().map(FileStatus::toString).collect(Collectors.joining(",\n    "));
  }
+
+  public Optional<FileStatus> getLatestChecksum() {


and I think these needs method docs? This checksum might not be for the version that this LogSegment represents, right?

also ... suppose we have 10.checkpoint and 11.json and 12.json ...

what are the possible ranges of the version for which this CRC could be for? Could we have 8.crc? what are the guarantees of this version range?

For logSegment, we just have 8.crc, downstream(LogReplay) decides use it or not.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/LogReplay.java

scottsand-db · 2025-04-28T21:02:29Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/LogReplay.java

  /** Returns the crc info for the current snapshot if the checksum file is read */
  public Optional<CRCInfo> getCurrentCrcInfo() {
-    return currentCrcInfo;
+    return cachedCrcInfo.filter(crc -> crc.getVersion() == logSegment.getVersion());


Also -- to clarify: the consumers of this API only ever want the CRCInfo if it was read during log replay? They never just want the CRCINfo for the latest version, even if it wasn't read during log replay? That semantic seems a bit odd to me -- can you please clarify / explain?

the consumer just try to get crcInfo of the current version. We return if it is cached. Updating the cache is not necessary during log replay's construction.

the consumer just try to get crcInfo of the current version.

But this API will only return it if it was read / cached?

My confusion is this:

it seems like consumers want the CRC info for the current version. full stop. if it exists, they want it

this API returns it if and only if it exists and we had to read it to update the snapshot hint

So, there seems to be a gap?

So the concern here is --- when version hint is used, then crcInfo will always empty. This was unfortunately the behavior before, although problematic.

I updated with a fix. (Previously it will cause CRC missing when snapshot hint is used, now it is getting worse as we load domain metadata, so agree to fix it along the way.)

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/LogReplay.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/LogSegment.java

huan233usc added 3 commits January 31, 2025 20:07

remove additional listing

dd75310

fix case log/checkpoint get deleted in iterator

866c4f7

fix bug of case

3dcd3c9

huan233usc self-assigned this Feb 3, 2025

huan233usc added 5 commits February 12, 2025 10:09

merge conflict

b0b3b45

fix test

b9df3f1

fix test

5823109

Merge branch 'master' into remove-listing

9c13890

flat map

96c4ff6

huan233usc marked this pull request as ready for review February 13, 2025 18:25

huan233usc requested review from allisonport-db, nicklan, scottsand-db and vkorukanti February 13, 2025 18:26

scottsand-db requested changes Feb 13, 2025

View reviewed changes

huan233usc added 2 commits February 13, 2025 16:14

rename some vars

aef6f9b

resolve

59327e0

huan233usc mentioned this pull request Mar 24, 2025

[Kernel] Track tech debt for next version of release #4282

Open

14 tasks

huan233usc added 12 commits March 24, 2025 15:09

merge

c70c436

scala

18ad7f9

fix

3a64ab8

fix

d6bb0ae

fix

ae5dad7

fix

13dc17c

fix

ab2eabb

fix

a7b9ae8

fix

a1893c2

fix

a421655

fix

1ceda70

fix

403c762

huan233usc requested a review from scottsand-db April 28, 2025 17:43

scottsand-db reviewed Apr 28, 2025

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/LogReplay.java Show resolved Hide resolved

scottsand-db reviewed Apr 28, 2025

View reviewed changes

huan233usc added 4 commits April 28, 2025 15:32

private class

3bb4b3f

merge

f9e6568

merge

7e199a4

doc

90b69ac

huan233usc requested a review from scottsand-db April 28, 2025 23:57

huan233usc added 8 commits April 28, 2025 21:42

fix

fc19e9f

fix

f217fd7

fix

5234983

fix

e482f4b

merge

4f90d83

fix

ba5e9a5

fix

57ad1a4

merge

2f4c231

scottsand-db reviewed May 1, 2025

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/LogReplay.java Show resolved Hide resolved

scottsand-db reviewed May 1, 2025

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/LogSegment.java Outdated Show resolved Hide resolved

scottsand-db reviewed May 1, 2025

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/LogSegment.java Outdated Show resolved Hide resolved

huan233usc added 5 commits May 1, 2025 14:33

fix

3e1bdd7

fix

ef92e34

fix comments

4e2a011

fix typo

f692b6a

fmt

ab53195

scottsand-db approved these changes May 1, 2025

View reviewed changes

huan233usc added 3 commits May 1, 2025 15:42

fmt

580a744

fmt

86f4211

update test

b2efd40

vkorukanti approved these changes May 2, 2025

View reviewed changes

vkorukanti merged commit 33e8666 into delta-io:master May 2, 2025
21 checks passed

[Kernel]Remove two file IO in CRC loading by reusing log listing #4112

[Kernel]Remove two file IO in CRC loading by reusing log listing #4112

Uh oh!

Conversation

huan233usc commented Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

huan233usc commented Feb 1, 2025 •

edited

Loading